PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian - #160441
Conversation
|
rustbot has assigned @JonathanBrouwer. Use Why was this reviewer chosen?The reviewer was selected based on:
|
|
@rustbot reroll |
To nitpick a bit here, the registers themselves have no endianness. However, the lane numbering in mnemonics is always "big-endian", i.e. counted from left to right, see e.g. the Vector/SIMD Multimedia Extension Technology Programming Environments Manual, section 1.2.2.1, "Byte Ordering". If we look at the Power ISA™ Version 3.0C specification, we see:
and if we look for example at LLVM is aware that these instructions are lane-sensitive, see llvm/llvm-project@77e34f1. However, the numbering used in the commit description differs from the ISA specification:
because LLVM's notion of lane numbering appears to refer to the logical vector lanes rather than the ISA's architectural element numbers. Therefore the change is correct. See for example the added test on real hardware without the fix: pub fn f64_to_f32(x: f64) -> f32 {
let res;
unsafe {
core::arch::asm!("xscvdpsp {}, {}", out(vsreg) res, in(vsreg) x, options(pure, nostack, nomem));
};
res
}
fn main() {
println!("{}", f64_to_f32(std::f64::consts::PI));
}currently on aelin@algol ~> lscpu | head -n 6
Architecture: ppc64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Big Endian
CPU(s): 40
On-line CPU(s) list: 0-39
Model name: POWER8 (architected), altivec supported
aelin@algol ~> rustc -Ctarget-feature=+vsx test.rs && ./test
warning: unstable feature specified for `-Ctarget-feature`: `vsx`
|
= note: this feature is not stably supported; its behavior can change in the future
warning: 1 warning emitted
3.1415927currently on aelin@fiora ~> lscpu | head -n 5
Architecture: ppc64le
Byte Order: Little Endian
CPU(s): 32
On-line CPU(s) list: 0-31
Model name: POWER9 (raw), altivec supported
aelin@fiora ~> rustc test.rs && ./test
0Thank you for the fix! |
There was a problem hiding this comment.
Some nits. I do really like the use of check-prefix, we should probably use that more, it's really useful in the LLVM repo.
Probably
r? @Amanieu
Also technically this is a breaking change then? Even though it is also a bug fix.
e1698ee to
e9b7e90
Compare
Technically yes, however since the previous behaviour didn't work at all with hardware instructions expecting a scalar (and any uses that just used hardware vector instructions instead will also work with the fixed behaviour) it seems unlikely any current usage will actually be broken by this bug fix. |
|
Right. I'll nominate for T-lang because it is technically a breaking change, but I don't see how this could be controversial at all and be anything but an immediate FCP (if that, maybe because this is really a bug fix we can even forego it). |
|
Makes sense to me. I propose to waive the 10-day final comment period (with all checkboxes). (I'm setting aside here whether it needs an FCP. It's safer to just FCP it than to risk us not getting to this in the meeting and thereby leaving it sit.) @rfcbot fcp merge lang |
|
@traviscross has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
|
@rfcbot reviewed |
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
|
I haven't dug into exactly the change, but it sounds like a bugfix to me -- if there's no way that it was ever possible or desired intended to put them into the current spot, I'd probably do this without an FCP. @rfcbot reviewed |
|
Registering that this should not be a lang matter; it seems like an architectural bugfix. We need to find a process by which we can stop having these matters come to lang. But for today: @rfcbot reviewed |
|
@rfcbot reviewed |
e9b7e90 to
fb0210d
Compare
|
@rustbot ready |
|
@bors try jobs=i686-gnu-nopt-1 |
This comment has been minimized.
This comment has been minimized.
PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian try-job: i686-gnu-nopt-1
|
Let's try this again @bors r=Amanieu |
…e, r=Amanieu PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian 64-bit PowerPC supports both big and little endian, however registers are always big endian. As a consequence of this, the order of vector lanes is reversed on little endian; however scalar `f32` and `f64` are always stored in the actual (big-endian) lane 0. This PR fixes the LLVM ASM fixup to take that into account. Ping target maintainers: @daltenty @gilamn5tr @amy-kwan @Gelbpunkt @famfo @neuschaefer
…e, r=Amanieu PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian 64-bit PowerPC supports both big and little endian, however registers are always big endian. As a consequence of this, the order of vector lanes is reversed on little endian; however scalar `f32` and `f64` are always stored in the actual (big-endian) lane 0. This PR fixes the LLVM ASM fixup to take that into account. Ping target maintainers: @daltenty @gilamn5tr @amy-kwan @Gelbpunkt @famfo @neuschaefer
…uwer Rollup of 11 pull requests Successful merges: - #158510 (Enable `static_position_independent_executables` on all gnu targets) - #160441 (PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian) - #160760 (Miri: give the incremental session a chance to finish) - #160892 (Inline some hot new-solver functions) - #160821 (Updates the expect messages in library/std/src/net/udp.rs to follow the style guide) - #160997 (Add an AGENTS.md that enforces the LLM policy) - #161057 (Silence unecessary errors on `Vec<foo::Bar>` to `Vec<foo:Bar>` typo) - #161079 (bootstrap: Clean up imports related to `define_config!` and `check_ci_llvm!`) - #161080 (propagate `RerunNonErased` in `projection_may_match`) - #161085 (bootstrap: Restrict the normalization of relative paths) - #161086 (add crashtests [3/N])
Rollup merge of #160441 - beetrees:inline-asm-fix-powerpc64le, r=Amanieu PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian 64-bit PowerPC supports both big and little endian, however registers are always big endian. As a consequence of this, the order of vector lanes is reversed on little endian; however scalar `f32` and `f64` are always stored in the actual (big-endian) lane 0. This PR fixes the LLVM ASM fixup to take that into account. Ping target maintainers: @daltenty @gilamn5tr @amy-kwan @Gelbpunkt @famfo @neuschaefer
Rollup merge of #160441 - beetrees:inline-asm-fix-powerpc64le, r=Amanieu PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian 64-bit PowerPC supports both big and little endian, however registers are always big endian. As a consequence of this, the order of vector lanes is reversed on little endian; however scalar `f32` and `f64` are always stored in the actual (big-endian) lane 0. This PR fixes the LLVM ASM fixup to take that into account. Ping target maintainers: @daltenty @gilamn5tr @amy-kwan @Gelbpunkt @famfo @neuschaefer
…uwer Rollup of 11 pull requests Successful merges: - rust-lang/rust#158510 (Enable `static_position_independent_executables` on all gnu targets) - rust-lang/rust#160441 (PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian) - rust-lang/rust#160760 (Miri: give the incremental session a chance to finish) - rust-lang/rust#160892 (Inline some hot new-solver functions) - rust-lang/rust#160821 (Updates the expect messages in library/std/src/net/udp.rs to follow the style guide) - rust-lang/rust#160997 (Add an AGENTS.md that enforces the LLM policy) - rust-lang/rust#161057 (Silence unecessary errors on `Vec<foo::Bar>` to `Vec<foo:Bar>` typo) - rust-lang/rust#161079 (bootstrap: Clean up imports related to `define_config!` and `check_ci_llvm!`) - rust-lang/rust#161080 (propagate `RerunNonErased` in `projection_may_match`) - rust-lang/rust#161085 (bootstrap: Restrict the normalization of relative paths) - rust-lang/rust#161086 (add crashtests [3/N])
Pkgsrc changes: * Adapt to changes in vendored crate versions. * Version & checksum changes. Upstream changes: Version 1.99.0 (2026-10-01) ========================== Language -------- - [Add allow-by-default `raw_borrows_via_references` lint that checks for references that decay immediately into raw borrows](rust-lang/rust#138230) - [Extend `unconditional_panic` lint to function calls that panic when the chunks/windows size is zero] (rust-lang/rust#153563) - [Stabilize C-variadic function definitions] (rust-lang/rust#155697) - [Stabilize the ability to use `#[unsafe(naked)]` functions to define C-variadic functions (`#![feature(c_variadic_naked_functions)]`).] (rust-lang/rust#159746) - [Trait methods are now resolved on an adjusted never type (producing a FCW)] (rust-lang/rust#156047) - [Coerce from inference variables to trait objects if the inference variable is related via subtyping to a type that is known to be `Sized`] (rust-lang/rust#157820) - [Stabilize `#[my_macro] mod foo;`] (rust-lang/rust#157857). This allows outlined modules (`mod foo;`) anywhere in the body of a custom attribute or derive macro. - [Fix the `overflowing_literals` lint with repeated negation] (rust-lang/rust#158302). For instance, it will now no longer lint on `--128_i8`, which is already detected by the `arithmetic_overflow` lint. - [Add POSIX symbols to the `invalid_runtime_symbol_definitions` and `suspicious_runtime_symbol_definitions` lints] (rust-lang/rust#158522) - [Lint unused `#[path]` attributes on inline modules] (rust-lang/rust#158835) - [Enable `unreachable_cfg_select_predicates` lint as part of `unused` lint group] (rust-lang/rust#159179) - [Stabilize passing 128-bit integers via vector registers with `asm!` on x86] (rust-lang/rust#159525) - [Explicitly document that some allocations are allowed to grow in-place (but none are allowed to shrink)] (rust-lang/rust#159729) - We now [guarantee] (rust-lang/rust#159730) that the contents of an `UnsafeCell` can be accessed without going through `get` - [The `invalid_reference_casting` lint was adjusted accordingly] (rust-lang/rust#159960) - [Account for globally enabled target features in `global_asm!`] (rust-lang/rust#160594) - [Warn if an invalid `doc` attribute is used on a macro invocation] (rust-lang/rust#161003) Compiler -------- - [Convert `-Ctarget-cpu` into a target-modifier for AVR, AMDGCN and NVPTX] (rust-lang/rust#150732) - [Enable `static_position_independent_executables` on all gnu and musl targets] (rust-lang/rust#158510) - When providing a suggestion about a missing method, rustc now prefers an exactly matching name from a [doc alias attribute] (https://doc.rust-lang.org/rustdoc/advanced-features.html#add-aliases-for-an-item-in-documentation-search) over a similarity search from other method names. If your new users sometimes expect a method under a different name, adding a doc alias will now help them find it via rustc suggestions, in addition to helping them find it via rustdoc search: [When suggesting method names, prefer *exact* doc aliases over similar names](rust-lang/rust#160369) Platform Support ---------------- - [Promote `riscv64-unknown-linux-musl` to Tier 2 with host tools] (rust-lang/rust#158766) Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support. [platform-support-doc]: https://doc.rust-lang.org/rustc/platform-support.html Libraries --------- - Iteration on `RangeInclusive` (`a..=b` ranges) is now [optimized better in some circumstances] (rust-lang/rust#155114). As a side effect of this, the behavior of `RangeInclusive` values that has already been exhausted (as an iterator) has changed. For example, the return values of `start()` and `end()` on such ranges may return different values, and using such ranges as slice indexes may have different behavior. These behaviors were not guaranteed to be stable, so these changes are considered to not be breaking changes. - [Relax `transmute_copy` to accept `?Sized` types] (rust-lang/rust#155989) - [Update `transmute_copy` to use a non-unwinding panic] (rust-lang/rust#155989) - [Don't escape U+FF9E and U+FF9F in `escape_debug_ext`] (rust-lang/rust#158057) - [Re-export `core::fmt::NumBuffer` in `alloc` (and `std`)] (rust-lang/rust#161430) Stabilized APIs --------------- - [`IntoIterator` for `Box<[T; N]>`] (https://doc.rust-lang.org/stable/std/iter/trait.IntoIterator.html#impl-IntoIterator-for-Box%3C%5BT;+N%5D,+A%3E) - [`IntoIterator` for `&Box<[T; N]>`] (https://doc.rust-lang.org/stable/std/iter/trait.IntoIterator.html#impl-IntoIterator-for-%26Box%3C%5BT;+N%5D,+A%3E) - [`IntoIterator` for `&mut Box<[T; N]>`] (https://doc.rust-lang.org/stable/std/iter/trait.IntoIterator.html#impl-IntoIterator-for-%26mut+Box%3C%5BT;+N%5D,+A%3E) - [`VecDeque::retain_back`] (https://doc.rust-lang.org/stable/std/collections/struct.VecDeque.html#method.retain_back) - [`core::ffi::VaList`] (https://doc.rust-lang.org/stable/core/ffi/struct.VaList.html) - [`Box::into_non_null`] (https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.into_non_null) - [`Box::from_non_null`] (https://doc.rust-lang.org/stable/std/boxed/struct.Box.html#method.from_non_null) - [`Vec::into_parts`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.into_parts) - [`Vec::from_parts`] (https://doc.rust-lang.org/stable/std/vec/struct.Vec.html#method.from_parts) - [`core::mem::size_of_val_raw`] (https://doc.rust-lang.org/stable/core/mem/fn.size_of_val_raw.html) - [`core::mem::align_of_val_raw`] (https://doc.rust-lang.org/stable/core/mem/fn.align_of_val_raw.html) - [`core::alloc::Layout::for_value_raw`] (https://doc.rust-lang.org/stable/core/alloc/struct.Layout.html#method.for_value_raw) - [`String::from_utf8_lossy_owned`] (https://doc.rust-lang.org/stable/std/string/struct.String.html#method.from_utf8_lossy_owned) - [`string::FromUtf8Error::into_utf8_lossy`] (https://doc.rust-lang.org/stable/std/string/struct.FromUtf8Error.html#method.into_utf8_lossy) - [`FusedIterator for StepBy<I>`] (https://doc.rust-lang.org/stable/std/iter/struct.StepBy.html#impl-FusedIterator-for-StepBy%3CI%3E) - [`std::fs::set_times`] (https://doc.rust-lang.org/stable/std/fs/fn.set_times.html) - [`std::fs::set_times_nofollow`] (https://doc.rust-lang.org/stable/std/fs/fn.set_times_nofollow.html) Cargo ----- - Add a new built-in profile `debug`. This is a preparation for transitioning the `dev` profile away from debugging to give a saner default for faster development iterations. Currently there is no difference between `dev` and `debug` profiles. [docs] (https://doc.rust-lang.org/nightly/cargo/reference/profiles.html#debug-1) [#17214] (rust-lang/cargo#17214) - Workspace members on edition 2024 or later can now override an inherited workspace dependency's `default-features` field. For example, `serde = { workspace = true, default-features = false }` now turns off default features even when the workspace definition enables them. On earlier editions, `default-features = false` is ignored with a warning. ([RFC 3945] (rust-lang/rfcs#3945)) [#17126](rust-lang/cargo#17126) - Incremental compilation is now disabled by default when running in CI. CI is detected via the CI environment variable. [#17220] (rust-lang/cargo#17220) See also the [full Cargo changelog] (https://doc.rust-lang.org/nightly/cargo/CHANGELOG.html#cargo-199-2026-10-01) Rustdoc ----- - [Add new `unused_footnote_definition` rustdoc lint] (rust-lang/rust#137858) - Smarter filtering of trait impls yields performance improvements of 20% on average and up to 40% on some real-world crates. ([1] (rust-lang/rust#159623), [2](rust-lang/rust#159721), [3](rust-lang/rust#159779), [4](rust-lang/rust#159854), [5](rust-lang/rust#159091)) Compatibility Notes ------------------- - [Fully deprecate the legacy integral modules] (rust-lang/rust#146882). For example, `std::i32::MAX` should be accessed via `i32::MAX` instead. - [Upgrade `no_mangle_generic_items` into hard error] (rust-lang/rust#154585) - [The `Pin::new_unchecked` has had its safety invariants changed slightly] (rust-lang/rust#156935) - [Do not promote references to extern statics] (rust-lang/rust#157641) - [Ensure that the inferred types of `let` patterns typecheck] (rust-lang/rust#157841) - [hermit/fs: Return `unsupported()` instead of `from_raw_os_error(22)`] (rust-lang/rust#158247) - [Fixed a bug where `#[repr(simd)]` was accidentally allowed on macro invocations on stable Rust] (rust-lang/rust#158523) - [Abort const-eval when there are generics in the type of the value being produced] (rust-lang/rust#159504) - [Attributes not applying to anything are now an error in code blocks in doc comments] (rust-lang/rust#159849) - [`Box::leak`: tell people to avoid unleaking] (rust-lang/rust#160323) - [PowerPC inline ASM: Fix scalar floats being in the wrong vector lane on little endian] (rust-lang/rust#160441) - [Do not take `doc(cfg())` into account when filtering doctests] (rust-lang/rust#159014) - [Infer anonymous lifetimes in the types of associated consts as `'static`] (rust-lang/rust#156508) - Macros that expand to a semicolon now produce a warning lint (`semicolon_in_expressions_from_non_local_macros`) even when the macro comes from another crate. Previously, such warnings only appeared for macros from the same crate, to avoid showing warnings that can't be fixed locally; however, this masked problems, as integration tests from the crate providing the macro get compiled as a separate crate, so tests often wouldn't reveal this issue. If you encounter a lint like this, please make sure to report it to the crate providing the macro so they can fix it; don't just silence it in your own crate. - [`semicolon_in_expressions_from_macros`: Lint on non-local macros too] (rust-lang/rust#159222) - [Split non-local `semicolon_in_expressions_from_macros` into a separate lint] (rust-lang/rust#159700) Internal Changes ---------------- These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools. - [Update to LLVM 23] (rust-lang/rust#158734)
View all comments
64-bit PowerPC supports both big and little endian, however registers are always big endian. As a consequence of this, the order of vector lanes is reversed on little endian; however scalar
f32andf64are always stored in the actual (big-endian) lane 0. This PR fixes the LLVM ASM fixup to take that into account.Ping target maintainers: @daltenty @gilamn5tr @amy-kwan @Gelbpunkt @famfo @neuschaefer